Initial ZendMM conversion of glob #17433
Conversation
|
About the persistence, it should not be persistent so it counts against memory limit which is the main point in changing this. But you should of course check all the usage and make sure it is never user outside the request or re-used between request - it shouldn't but better to check. |
|
I'm also not sure about the glob limits (for malloc and others). It might be good to check if glibc have similar limits to see if they should be kept - doesn't need to be changed as part of this PR but would make sense to look into before switching to it on Linux as well. |
NattyNarwhal
force-pushed
the
glob-zendmm
branch
from
03ccf37 to
9488a27
Compare
In the places glob is used: ...all instances of it call globfree in the same function, with the exception of the glob stream wrapper. |
|
Looking at this PR again, I think not allocating glob memory as persistent is safe as long as there's no way to store a glob stream wrapper across request boundaries. I can't think of any off the top of my head... |
Ideally temporary until phpGH-17433.
Ideally temporary until phpGH-17433.
Ideally temporary until phpGH-17433.
* Move glob to main/ from win32/ In preparation to make the Win32 reimplementation the standard cross-platform one. Currently, it doesn't do that and just passes through the original glob implementation. We could consider also having an option to use the standard glob for systems that have a sufficient one. * Enable building with win32 glob on non-windows Kind of broken. We're namespacing the function and struct, but not yet the GLOB_* defines. There are a lot of places callers check if i.e. NOMATCH is defined that would likely become redundant. Currently it also has php_glob and #defines glob php_glob (etc.) - I suspect doing the opposite and changing the callers would make more sense, just doing MVP to geet it to build (even if it fails tests). * Massive first pass at conversion to internal glob Have not tested yet. the big things are: - Should be invisible to userland PHP code. - A lot of :%s/GLOB_/PHP_GLOB_/g; the diff can be noisy as a result, especially in comments. - Prefixes everything with PHP_ to avoid conflicts with system glob in case it gets included transitively. - A lot of weird shared definitions that were sprawled out to other headers are now included in php_glob.h. - A lot of (but not yet all cases) of HAVE_GLOB are removed, since we can always fall back to php_glob. - Using the system glob is not wired up yet; it'll need more shim ifdefs for each flag type than just glob_t/glob/globfree defs. * Fix inclusion of GLOB_ONLYDIR This is a GNU extension, but we don't need to implement it, as the GNU implementation is flawed enough that callers have to manually filter it anyways; just provide a stub definition for the constant. We could consideer implementing this properly later. For now, fixes the basic glob constant tests. * Remove HAVE_GLOBs We now always have a glob implementation that works. HAVE_GLOB should only be used to check if we have a system implementation, for if we decide to wrap the system implementation instead. * We don't need to care about being POSIXly correct for internal glob * Check for reallocarray Ideally temporary until GH-17433. * Forgot to move this file from win32/ to main/ * Check for issetugid (BSD function) * Allow using the system glob with --enable-system-glob * Style fix after removing ifdef * Remove empty case for system glob
Removes the vendored OpenBSD reallocarray and changes libc memory management to ZendMM. Not sure if non-persistent is the right call, I think glob users use it then call globfree ASAP within a request, but it might be preferable to use persistent allocations.
|
Rebased this after GH-18164 was merged. Also let me know if this looks good, it makes the interim |
Removes the vendored OpenBSD reallocarray and changes libc memory management to ZendMM.
Not sure if non-persistent is the right call, I think glob users use it then call globfree ASAP within a request, but it might be preferable to use persistent allocations.